Skip to content

stream: allow null as second arg in Transform callback#62803

Open
galaxy4276 wants to merge 2 commits intonodejs:mainfrom
galaxy4276:stream/fix-transform-callback-null
Open

stream: allow null as second arg in Transform callback#62803
galaxy4276 wants to merge 2 commits intonodejs:mainfrom
galaxy4276:stream/fix-transform-callback-null

Conversation

@galaxy4276
Copy link
Copy Markdown

@galaxy4276 galaxy4276 commented Apr 18, 2026

What

The docs say passing a value as the second argument to the transform callback is equivalent to calling transform.push(value). That implies callback(null, null) should push null — ending the readable side — just like this.push(null); callback() does. It doesn't.

Bug

const { Transform } = require('stream');

const t = new Transform({
  transform(chunk, encoding, callback) {
    callback(null, null); // intend to signal EOF
  },
});

t.on('end', () => console.log('ended')); // never fires
t.write('hello');
t.end();

The 'end' event never fires. The stream hangs.

Root cause — lib/internal/streams/transform.js:

// Before
if (val != null) {   // loose equality: null == null → true → blocks push(null)
  this.push(val);
}

val != null uses loose equality, so both null and undefined fail the check. this.push(null) is never called, state.ended stays false, and 'end' is never emitted.

Fix

// After
if (val !== undefined) {  // only block undefined; null passes through
  this.push(val);
}

Now callback(null, null) reaches this.push(null), which sets state.ended = true and eventually emits 'end' once the buffer drains — matching what the docs describe.

Behavior after fix

const t = new Transform({
  transform(chunk, encoding, callback) {
    callback(null, null); // equivalent to: this.push(null); callback()
  },
});

t.on('end', () => console.log('ended')); // fires correctly now
t.write('hello');
t.end();

Potential concern

This is technically a behavior change. Code that relied on callback(null, null) not ending the stream should use callback() (no second argument) instead. I think this is the right call given the docs, but open to pushback — especially on whether this warrants a semver-minor label or a deprecation note before changing.

@galaxy4276 @claude
stream: allow null as second arg in Transform callback
8aef7e6 
callback(null, null) in a Transform._transform method should be
equivalent to calling this.push(null) followed by callback(), ending
the readable side of the stream. Previously val != null blocked the
push because null == null is true in loose equality, so push(null)
was never called and the stream never emitted 'end'.

Change the guard from `val != null` to `val !== undefined` so that
null passes through to this.push(), which sets state.ended and
eventually emits 'end', matching documented behavior.

Fixes: nodejs#62769

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Apr 18, 2026

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Apr 18, 2026
@galaxy4276
Copy link
Copy Markdown
Author

galaxy4276 commented Apr 18, 2026

Looking at the history, val != null has been in transform.js since the file moved to internal/streams (~2020), so this isn't a regression — it's been silently broken for a long time. The loose equality check was likely written to block undefined (when no second arg is passed), but it incidentally also blocks null, which is the EOF sentinel.

The fix changes to val !== undefined, which only blocks undefined and lets null through to this.push(null) — matching what the docs describe:

"If a second argument is passed to the callback, it will be forwarded on to the transform.push() method"

In practice, callback(null, null) to signal EOF is uncommon — most users write this.push(null); callback() — so the blast radius of this change should be small. There's also no legitimate use case for callback(null, null) meaning "push nothing and continue", since null is the EOF sentinel in both buffer and object mode.

That said, I'm uncertain whether this is semver-patch (bug fix aligning with docs) or semver-minor (observable behavior change). Would appreciate guidance on the right label and whether a CHANGELOG note is needed.

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 18, 2026
@ronag ronag requested a review from mcollina April 18, 2026 14:44
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Apr 18, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/72759/

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 18, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.69%. Comparing base (31b9e60) to head (4aefb07).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #62803      +/-   ##
==========================================
- Coverage   89.70%   89.69%   -0.01%     
==========================================
  Files         706      706              
  Lines      218288   218222      -66     
  Branches    41782    41767      -15     
==========================================
- Hits       195806   195742      -64     
+ Misses      14394    14381      -13     
- Partials     8088     8099      +11
Files with missing lines Coverage Δ
lib/internal/streams/transform.js 98.52% <100.00%> (ø)

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@galaxy4276 @claude
test: fix capitalized-comments lint in transform callback null test
4aefb07 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@galaxy4276
Copy link
Copy Markdown
Author

galaxy4276 commented Apr 18, 2026

@lpinca @ronag The only change since your review is fixing a capitalized-comments lint error on line 21.
Would you mind re-approving?

@galaxy4276 galaxy4276 requested review from lpinca and ronag April 18, 2026 19:35
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2026
mcollina
mcollina requested changes Apr 18, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a pass of citgm before landing.

I expect a significant breakage in the ecosystem.

@galaxy4276
Copy link
Copy Markdown
Author

galaxy4276 commented Apr 19, 2026

This should have a pass of citgm before landing.

I expect a significant breakage in the ecosystem.

Thank you for your review.
As expected, since I'm changing the internal implementation, testing will be necessary.
I'll conduct stability testing through citgm and attach the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina requested changes

@lpinca lpinca Awaiting requested review from lpinca

@ronag ronag Awaiting requested review from ronag

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@galaxy4276 @nodejs-github-bot @mcollina @lpinca @ronag